Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace StringBuffer with StringBuilder #243

Merged
merged 1 commit into from Jan 8, 2015

Conversation

alexismeneses
Copy link
Contributor

Take advantage of Java 5 using StringBuilder where applicable

  • replacement is done when StringBuffer is used as a local variable
  • public methods using StringBuffer are kept but deprecated in favor of the same using StringBuilder. Cases can be found in org.postgresql.core.Utils. New public methods using StringBuilder do not use the same name as their StringBuffer counterpart otherwise it would prevent existing code using null as a method argument to compile.

…der where applicable

 - replacement is done when StringBuffer is used as a local variable
 - public APIs using StringBuffer are kept but deprecated in favor of the same using StringBuilder (note a method name change is required not to prevent code using null as parameter to compile)
@sehrope
Copy link
Member

sehrope commented Jan 6, 2015

First off I think this is awesome. I've been thinking about this every time I poke through the code. Nice to see someone got around to it.

I don't see a point to maintaining two copies of the public methods. They may be public, but they're still internal to the driver. The only true public API of the driver is the core JDBC API and the extensions available via casting to PGConnection (ex: the CopyManager API).

Nobody should be using the "internal" public methods (ex: the Utils class) and, if they are, it'd be their responsibility to update their code to use a StringBuilder instead. I say this as someone who has used those internal helper functions in the past.

This is especially true if it'll be done as a new major release of the driver. Users that are upgrading to a 9.4 (or I guess 9.5?) driver can be expected to compile/test it with their app. It'll immediately be apparent what they need to fix.

@davecramer @ringerc : What do you guys think?

@davecramer
Copy link
Member

Ya, I see no reason to keep the old methods around.

Dave Cramer

On 5 January 2015 at 21:24, Sehrope Sarkuni notifications@github.com
wrote:

First off I think this is awesome. I've been thinking about this every
time I poke through the code. Nice to see someone got around to it.

I don't see a point to maintaining two copies of the public methods. They
may be public, but they're still internal to the driver. The only true
public API of the driver is the core JDBC API and the extensions available
via casting to PGConnection (ex: the CopyManager API).

Nobody should be using the "internal" public methods (ex: the Utils class)
and, if they are, it'd be their responsibility to update their code to use
a StringBuilder instead. I say this as someone who has used those internal
helper functions in the past.

This is especially true if it'll be done as a new major release of the
driver. Users that are upgrading to a 9.4 (or I guess 9.5?) driver can be
expected to compile/test it with their app. It'll immediately be apparent
what they need to fix.

@davecramer https://github.com/davecramer @ringerc
https://github.com/ringerc : What do you guys think?


Reply to this email directly or view it on GitHub
#243 (comment).

@alexismeneses
Copy link
Contributor Author

Just note that the real logic is done in a shared private method. Public methods just delegate the job to it whether the provided arg is a StringBuilder or StringBuffer. So there not really two copies to maintain actually.

davecramer added a commit that referenced this pull request Jan 8, 2015
Replace StringBuffer with StringBuilder for performance
@davecramer davecramer merged commit 5060291 into pgjdbc:master Jan 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants